I have found that the local copy of cpu/atmega_common/avr-libc-extra/time.h conflicts with the system avr-libc, and makes the time.h functions produce garbage output. Since this was actually used almost nowhere, I removed it. There was a minor struct that is not provided by avr-libc time.h, which I moved into cpu/atmega_common/include/sys/time.h. I can split this off into another PR, if you think it is necessary.

I have fixed all of the problems listed in my first comment, except for the lagged messages. There is one final tricky bug I am trying to track down: there is a race condition that is related to use of xtimer_usleep that I have not been able to fix. When xtimer is disabled, everything is fine; when xtimer is enabled and debug is enabled, everything is fine; when only xtimer is enabled, some checks hang and alarms never trigger.

time.h change is now #8857

Regarding using RTC as a wrapper for RTT, see also:

• https://github.com/RIOT-OS/RIOT/blob/master/cpu/kinetis/periph/rtc.c
• kinetis/rtc: move to periph_common? #4565
• kinetis/rtc: move rtt-to-rtc wrapper to drivers/periph_common #4572

Unlike a simple RTC wrapper like in cpu/kinetis, this implementation can be used concurrently to the RTT. This is because it keeps time independently of the RTT, and it utilizes a different counter channel on the timer.

I have found that the xtimer_usleep problem is an instance of #6526 and not directly related to the code here. The hang on tests/periph_rtc is solved by turning off xtimer, or adding a while(1) to the end of main. It seems to be the same problem every other interrupt has after exiting main.

I don't think there are any remaining outstanding issues. As soon as #8857 merges, it should be ready for CI build testing.

The xtimer_usleep problem is fixed by #8904 for tests/periph_rtt, and thus is probably an instance of #8896.

Unfortunately, there seems to be another race condition either added by #8904, or uncovered by it, that prevents tests/periph_rtc from working.

It seems that moving rtc_init conflicts with the organization of the init systems (see #8914). It is also placed early enough that xtimer cannot be used. Swapping threads is not going to be useful because other threads have not been started (it is before kernel_init). Is it acceptable in this case to spin for 30-60ms while waiting for the busy flags to clear, or is there another alternative? This is required by the hardware for safe access to the asynchronous registers.

OK, I briefly skimmed the ATMega1284p data sheet to find out more about the hardware.
Assumptions:

• From the code, it seems like TOSC1 clock signal is assumed coming from a 32768 Hz RTC crystal.
• We want a relatively high prescaler on that clock source so that we don't need to service the overflow interrupt too often, since the timer/counter is only 8 bits wide.

From the data sheet:

When writing to one of the registers TCNT2, OCR2x, or TCCR2x, the value is transferred to a
temporary register, and latched after two positive edges on TOSC1. The user should not write
a new value before the contents of the temporary register have been transferred to its
destination. Each of the five mentioned registers have their individual temporary register, which
means that e.g. writing to TCNT2 does not disturb an OCR2x write in progress. To detect that a
transfer to the destination register has taken place, the Asynchronous Status Register – ASSR
has been implemented.

My conclusion from that text is that the synchronization takes 1 < t <= 2 [TOSC1 ticks], which is running at 32768 Hz, meaning that the syncronization is between 30.5 µs and 61 µs. The nomenclature in the rest of that chapter is that TOSC1 is the input to the prescaler, and not the divided counter signal.

Please tell me if you have any indication that the synchronization is indeed using the divided clock signal instead of the unscaled one. To me it seems like a pretty bad hardware design choice to let the silicon take 1/16 second to read/write a value from a timer. AFAICT using TOSC1 directly instead of the divided signal would not introduce any asynchronous problems since the TOSC1 signal and the scaled clock signal are synchronous compared to each other, since they are derived from the same signal, albeit with some small, non-zero, phase shift because of propagation delays in the prescaler.

If my conclusions are true, you should drop the xtimer calls completely and just spin for the sync flags to clear, 60 µs is definitely OK to spin while setting a slow 32 Hz timer target in my opinion. It is worth mentioning this spin in the documentation or code comments somewhere so that developers can find out about this without studying the details of the data sheet. I suggest putting it in the documentation block for the __asynch_wait function.

Also, please consider any possible race conditions when using the RTC and RTT from two different threads. It may be necessary to mask interrupts in some parts of the low level hardware accesses to prevent clobbering each others settings, especially in read-modify-write situations.

@gebart Sorry for my confusion. I thought I had seen an entry indicating that the synchronization unit used the prescaler and not TOSC1 directly, but I was mistaken.

__asynch_wait now spins, and I put a comment on about how long that will take. The only remaining problem is solved by #8857. Let me know when you want me to squash.

There shouldn't be any race conditions with using RTT and RTC together since the RTC does not actually change any RTT settings. The alarm used by the RTC is handled by counter B, while the alarm used by the RTT is counter A. So long as nothing runs rtt_poweroff the worst that can happen is a 0-8 glitch in timing when running rtt_set_counter while the RTC is running, but this should not prevent any ISRs from firing properly (just delay them). I suppose I could add some code to compensate for the timing glitch, but that would require editing RTC variables in the RTT code.

@ZetaR60 I don't think you need to handle that case here and now, I just wanted to make sure you had considered what the potential races are in this implementation.

I just realized something, you need to rename __async_wait, double underscores are forbidden in identifiers in C, outside of the compiler provided macros and support libs

Changed to _asynch_wait. I was a bit confused about single and double underscores, considering the usage in e.g. cpu/atmega_common/thread_arch.c. Am I correct in thinking that single underscores are preferred for naming static functions in RIOT libraries?

Also ran uncrustify.

Squashed with requested changes incorporated.

@ZetaR60 it is easier to review the PR if you don't squash your changes until you get a request to squash by a maintainer. Otherwise the person reviewing will need to try to remember what the code was like before. Making reviewing easy helps getting your PR merged more quickly.

@gebart Sorry. I thought that the amount of time waited was enough. I managed to unsquash from a backup.

ping @kaspar030 @gebart now that the release time crunch is over.

@gebart Is there anything else that this PR needs?

@jcarrano

Now, @ZetaR60, what is not clear to me is whether the RTT/RTC functions are safe to call from within an interrupt. Reading the code suggests it's not as the overflow would not get handled immediately. If that is correct, it should be documented as a limitation.

My latest commits should fix this issue. I believe that all code is now safe from timer events, other ISRs interrupting the current thread, and running the code with interrupts disabled or in ISR mode. It is not safe from concurrent access by multiple threads, except in the case where only state reads are occurring. However, so far as I can tell this is not generally expected of RIOT APIs, and it can be made concurrent access safe by disabling ISRs during access (like everything else).

It seems that making the code safe from concurrent access by multiple threads was easier than I thought. I think I have handled all of the various conditions that can occur; this is fairly difficult though, since there is multiple asynchronous considerations at almost every line of code, so I may have missed some things.

@maribu @jcarrano et al: I would like to squash and rebase as the list of changes is getting quite messy and out of date. Please let me know if this is acceptable.

I would like to squash and rebase

I'd say go ahead.

Is there a TL;DR for the discussion?
Maybe it would be easier to have separate PRs for RTT and RTC?

A rebase would be in order any way.

When I run this on arduino-mega2560, the tests/periph_rtt test prints (with ENABLE_DEBUG=1)

2019-11-05 17:50:30,220 # Initializing the RTT driver
2019-11-05 17:50:30,221 # Initializing RTT
2019-11-05 17:50:30,239 # RTT waits until ASSR not busy
2019-11-05 17:50:31,412 # RTT initialized
2019-11-05 17:50:31,435 # RTT sleeps until safe to read TCNT2
2019-11-05 17:50:33,227 # RTT now: 0
2019-11-05 17:50:33,260 # Setting initial alarm to now + 5 s (160)
2019-11-05 17:50:33,303 # RTC sleeps until safe read TCNT2 and to write OCR2B
2019-11-05 17:50:33,552 # RTT set alarm TCNT2: 0, OCR2A: 160
2019-11-05 17:50:33,553 # RTT alarm interrupt active
2019-11-05 17:50:33,619 # Done setting up the RTT, wait for many Hellos
2019-11-05 17:50:33,638 # RTT sleeps until safe to read TCNT2
2019-11-05 17:50:33,835 # (RTT now: 0)

I'm a bit surprised by how long it stays in the _asynch_wait() loop.

How do these crystals work?
On my Mega 2560 Pro / arduino-mega2560 I have two crystals: 12 MHz and 16 MHz and the RTT won't tick.

I now have a rss2-mega256rfr2 with a 32kHz XTAL and RTT works right out of the box with this PR.

@benpicco I think board is missing the 32kHz xtal needed for TIM2. This timer can be asynchronous and work even if we put power hungry timers in sleep mode. As a fallback for RDC is essential. But challenge is to sync the clocks.
The tests/periph_rtc seems not compatible as the update interval is 8s while the alarm is set to 2s in the test program.

PING everyone, should we get this done?

RTT support has been merged now, should be possible to base RTC support off that if you set RTT_FREQUENCY to 32 Hz (which appears to be the minimal frequency)

Maybe we could also just provide the RTC interface on top of the RTT? Unless the hardware internationally uses years, hours, minutes and seconds as units, I would consider the RTT interface the better option. And if one really wants an RTC like interface, a generic wrapper on top of RTT would solve this for all clock hardware (except those really internally using the broken up time / date fornat).

Maybe we could also just provide the RTC interface on top of the RTT?

Something like this?

int rtc_set_time(struct tm *time)
{
    uint32_t t = RTT_SEC_TO_TICKS(mktime(time));
    rtt_set_counter(t);

    return 0;
}

int rtc_get_time(struct tm *time)
{
    time_t t = RTT_TICKS_TO_SEC(rtt_get_counter());
    gmtime_r(&t, time);

    return 0;
}

int rtc_set_alarm(struct tm *time, rtc_alarm_cb_t cb, void *arg)
{
    time_t t = mktime(time);
    rtt_set_alarm(RTT_SEC_TO_TICKS(t), cb, arg);

    return 0;
}

int rtc_get_alarm(struct tm *time)
{
    time_t t = RTT_TICKS_TO_SEC(rtt_get_alarm());
    gmtime_r(&t, time);

    return 0;
}

(We can replace the POSIX functions with rtc_mktime() and rtc_localtime() from #13284 for smaller code and custom range)

Something like this?

There's already something like this somewhere in the codebase at least here: cpu/kinetis/periph/rtc.c.

The RTT overflows too fast to use it like that (the slowest is 32 Hz and it only has 24 bit), but we can make use of rtc_tm_normalize() from periph_common/rtc.c.

Both RTT and RTC support have now been merged.